wai-app-static: use cryptonite instead of cryptohash #519
Conversation
| @@ -120,8 +119,8 @@ webAppLookup hashFunc prefix pieces = | |||
| -- exists. | |||
| hashFile :: FilePath -> IO ByteString | |||
| hashFile fp = do | |||
| h <- Crypto.Hash.Conduit.hashFile fp | |||
| return $ B64.encode $ toBytes (h :: Digest MD5) | |||
| f <- BL.readFile fp | |||
There was a problem hiding this comment.
This has the potential to leak FDs. A better approach would be withBinaryFile, which will guarantee that the FD is closed.
There was a problem hiding this comment.
Lazy ByteString's readFile closes the FD ("The Handle will be held open until EOF is encountered"). I've tried withBinaryFile, it results in hGetBufSome: illegal operation (handle is closed)
There was a problem hiding this comment.
The correct way to do this is something like:
withBinaryFile fp ReadMode $ \h -> do
lbs <- L.hGetContents h
...The problem with readFile rears its head in the case of async exceptions. It's an uncommon case to be sure, but I try to avoid the lazy I/O functions in libraries anyway to prevent confusing behavior.
|
The Travis build did not pass, can you resolve that before merge? Otherwise, the change seems good to me. |
|
Works fine now with lower dependency bounds. |
|
Oh – it was closed because the whole function was lazy. Updated to use |
wai-app-static: use cryptonite instead of cryptohash
|
Thanks! |
cryptohashis deprecated in favor ofcryptonite. Not marked as such on Hackage yet, only on GitHub. Buttlsusescryptonitealready, so these two packages already clash (they export the exact same API).Also,
memoryinstead ofbase64-bytestringfor Base64 encoding (memoryis a dependency ofcryptonite).Yes,
conduitis also dropped, becausecryptohash-conduitwasn't updated in a long time, and wasn't ported tocryptonite. I think it's perfectly fine to use lazy I/O here, for the sake of fewer dependencies.